-
Notifications
You must be signed in to change notification settings - Fork 455
Round inputs for dense unrolled RNN tests to make pytests more stable #1284
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Round inputs for dense unrolled RNN tests to make pytests more stable #1284
Conversation
@@ -107,6 +107,7 @@ def test_resource_unrolled_rnn(rnn_layer, backend, io_type, static, reuse_factor | |||
# Subtract 0.5 to include negative values | |||
input_shape = (12, 8) | |||
X = np.random.rand(50, *input_shape) - 0.5 | |||
X = np.round(X * 2**16) * 2**-16 # make it exact ap_fixed<32,16> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is indeed ap_fixed<17, 0>
. Though, as you mentioned that the issue is more prevalent with unrolled dense, is there anything compromising bit-exactness between implementations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be the RNN, or maybe just LSTM, in general. Jovan did this change to get the tests to behave better in the pytorch parser case. It seemed to have worked there, so I'd be in favor of merging this now so get more meaningful test results, and look into why this is such an issue later.
This issue with the RNN precision is still causing pytests to fail in other PRs, btw. So I would still be in favor of merging this so that we don't get as many spurious test failures. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rest of the codebase uses this, so we can continue with it until a more universal solution comes up
As the error we are looking at is ~0.05, and this PR reduces the input error by ~1e-5, I'm a bit worried if this will actually patch the issue. I assumed that setting default in #1215 is the major reason for fixing the issue, though in this case it is already there. |
I could never reproduce this issue locally, so I'm not sure if this will actually fix the tests. But a similar fix helped with the same issue for the torch RNN tests, so the hope is that it will help. |
We are still having problems with numerical stability of RNNs in the pytests for dense unrolled. This PR aims to fix that by implementing the rounding @jmitrevs added in #1215 for these tests as well. As I haven't been able to reproduce the failure locally, I couldn't check if this actually fixes it
Type of change
Tests
Checklist
pre-commit
on the files I edited or added.